Don't set a source model on the attribute when it's not needed. This … #18244
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…avoids accidentally persisting the source model to the database when using multiselect attributes.
Description
This removes calling a setter (
setSourceModel
) inside a getter (getSource
) which is considered bad practice, since it changes state of an object, and a getter shouldn't change the state of an object.This is not some theoretical case, please see the manual testing scenario or #13156 to have an example where this can be seen in practice.
Fixed Issues (if relevant)
Manual testing scenarios
abcd
(see step 6)multiselect
product attribute, use attribute code:test_multi
eav_attribute
, notice that thesource_model
isNULL
source_model
is stillNULL
in the databasesource_model
staysNULL
, but it doesn't, it changes toMagento\Eav\Model\Entity\Attribute\Source\Table
This PR fixes this.
Further
The reason why this only happens with
multiselect
attributes and not withselect
attributes, is because of this piece of code: https://github.com/magento/magento2/blob/f36db11/app/code/Magento/Catalog/Model/ResourceModel/Eav/Attribute.php#L383-L392I don't know if
multiselect
should be added as well next to the condition on theselect
in that piece of code. I didn't try to figure it out anymore, since it was already complex enough to figure out all what was going on :)Contribution checklist